feat(Compute): Use paginated lists. by amanda-tarafa · Pull Request #1445 · GoogleCloudPlatform/dotnet-docs-samples

amanda-tarafa · 2021-08-10T11:48:10Z

Updates Google.Cloud.Compute.V1 dependency.
Closes #1438

jskeet · 2021-08-10T11:51:56Z

-            Zone = zone,
-        };
-        do
+        await foreach(var instance in client.ListAsync(projectId, zone))


An alternative to await foreach here would be to await client.ListAsync(projectId, zone).ToListAsync() to come up with allInstances, then synchronously iterate over that, but I'm not bothered either way.

This one gives you he chance to break in the middle of the async iteration without having fetched everything, right? Even if we are not showing that,...

Yes, it does. (And will show results as it goes, of course.) I just twitch at manually adding to the list ;)

Ah yes, that's for the purposes of the tests only, but I do see what you mean about that making you twitch. We have several tests that do this kind of thing across the repo. It's the alternative to testing against stdout.

snippet-bot · 2021-08-11T17:48:32Z

Here is the summary of changes.

You are about to add 2 region tags.

compute/api/Compute.Samples/ListImagesAsync.cs:17, tag compute_images_list
compute/api/Compute.Samples/ListImagesPerPageAsync.cs:17, tag compute_images_list_page

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

Refresh this comment

amanda-tarafa · 2021-08-11T17:50:31Z

@rsamborski I've added the two new samples, PTAL. Thanks.

Sita04 · 2021-08-11T21:34:02Z

+        // Make the request to list all non-deprecated images in a project.
+        ListImagesRequest request = new ListImagesRequest
+        {
+            Project = projectId,


MaxResults = 2,
Kindly add MaxResults parameter, which tells the number of images to be displayed in each page.
If not added, all images will be displayed in a single page.

If not added, all images will be displayed in a single page.

Not really, MaxResults has a default value of 500 as specified on API ref docs

That said, I'm completely fine with adding an explicit MaxResults for demonstration purposes but I don't think it should have a value of 2, this is a very low page size value, and will provoke plenty of requests to the server for anyone listing even a few images.
I'm not even worried about our sample tests, I'm worried about users copying this code as is and including it in their projects, and then generating all this unnecessary trafic for their own application.
I'll set to 100, and I'm happy to adjust it some more. Also, please, consider adjusting this value to something far greater than 2 for samples in other languages as well.

We used 10 in PHP and Python for the paginated results as this seemed to be a good number for demonstration purposes.

Yes 10 seems better, I did list sevaral of the public image sets and couldn't find any set containing more than 200 non-deprecated images. I'll change here to 10, but leave the fully automatic one on 100.

Thanks Amanda for the explanation! I understand the rational behind the param setting. Will double check the param values in other languages as well.

Sita04 · 2021-08-11T21:34:48Z

+        // Make the request to list all non-deprecated images in a project.
+        ListImagesRequest request = new ListImagesRequest
+        {
+            Project = projectId,


MaxResults = 2,
Kindly add MaxResults parameter, which tells the number of images to be displayed in each page.

Same response as below, I've set it to MaxResult = 100.

I agree with 100 here, because we are not showing results page by page and just using this under the hood to generate multiple requests.

Sita04 · 2021-08-11T22:01:19Z

+            // Listing only non-deprecated images to reduce the size of the reply.
+            Filter = "deprecated.state != DEPRECATED"
+        };
+        // Calling AsRawResponses will return a sequence of pages.


Suggested change

// Calling AsRawResponses will return a sequence of pages.

// Use the `AsRawResponses` attribute of returned iterable to have more granular control of

// iteration over paginated results from the API. Each time you want to access the

// next page, the library retrieves that page from the API.

Suggesting the comment for standardisation across languages.

Done, but changed slightly for idiomacity:

AsRawResponses is a method not a property (or attribute in Java), so it's more accurate to say Call the AsRawResponse() method...

iterable => sequence

distinguishing between the first sequence and the second. The first is the images sequence and the second is the page sequence.

I've change it to:

// Call the AsRawResponses() method of the returned image sequence to access the page sequece instead // This allows you to have more granular control of iteration over paginated results from the API. // Each time you request the next element on the page sequence, the library retrieves that page from the API.

Thank you for changing.

Sita04 · 2021-08-11T22:03:19Z

+            // Listing only non-deprecated images to reduce the size of the reply.
+            Filter = "deprecated.state != DEPRECATED"
+        };
+        await foreach (var image in client.ListAsync(request))


// Although the `MaxResults` parameter is specified in the request, the iterable returned // by the `ListAsync()` method hides the pagination mechanic. The library makes multiple // requests to the API for you, so you can simply iterate over all the images.

Kindly add the above comment. Thanks!

Done, with a slight word change, becuse it's more idiomatic: iterable => sequence

Sita04

Thanks Amanda!

rsamborski

Thanks Amanda. LGTM once Sita's comments are addressed.

amanda-tarafa

Review feedback addressed, PTAL @Sita04 , thanks!

amanda-tarafa · 2021-08-12T16:54:12Z

+        // Make the request to list all non-deprecated images in a project.
+        ListImagesRequest request = new ListImagesRequest
+        {
+            Project = projectId,


If not added, all images will be displayed in a single page.

Not really, MaxResults has a default value of 500 as specified on API ref docs

That said, I'm completely fine with adding an explicit MaxResults for demonstration purposes but I don't think it should have a value of 2, this is a very low page size value, and will provoke plenty of requests to the server for anyone listing even a few images.
I'm not even worried about our sample tests, I'm worried about users copying this code as is and including it in their projects, and then generating all this unnecessary trafic for their own application.
I'll set to 100, and I'm happy to adjust it some more. Also, please, consider adjusting this value to something far greater than 2 for samples in other languages as well.

amanda-tarafa · 2021-08-12T16:56:13Z

+        // Make the request to list all non-deprecated images in a project.
+        ListImagesRequest request = new ListImagesRequest
+        {
+            Project = projectId,


Same response as below, I've set it to MaxResult = 100.

amanda-tarafa · 2021-08-12T17:01:09Z

+            // Listing only non-deprecated images to reduce the size of the reply.
+            Filter = "deprecated.state != DEPRECATED"
+        };
+        await foreach (var image in client.ListAsync(request))


Done, with a slight word change, becuse it's more idiomatic: iterable => sequence

amanda-tarafa · 2021-08-13T08:05:33Z

+            // Listing only non-deprecated images to reduce the size of the reply.
+            Filter = "deprecated.state != DEPRECATED"
+        };
+        // Calling AsRawResponses will return a sequence of pages.


Done, but changed slightly for idiomacity:

AsRawResponses is a method not a property (or attribute in Java), so it's more accurate to say Call the AsRawResponse() method...

iterable => sequence

distinguishing between the first sequence and the second. The first is the images sequence and the second is the page sequence.

I've change it to:

// Call the AsRawResponses() method of the returned image sequence to access the page sequece instead // This allows you to have more granular control of iteration over paginated results from the API. // Each time you request the next element on the page sequence, the library retrieves that page from the API.

Updates Google.Cloud.Compute.V1 dependency. Closes GoogleCloudPlatform#1438

Sita04 · 2021-08-13T18:20:17Z

Thanks for making the changes, @amanda-tarafa! LGTM

amanda-tarafa · 2021-08-16T10:39:58Z

Thanks! Failures in CI are unrelated, will force merge.

amanda-tarafa requested review from jskeet and rsamborski August 10, 2021 11:48

amanda-tarafa self-assigned this Aug 10, 2021

product-auto-label Bot added the api: compute Issues related to the Compute Engine API. label Aug 10, 2021

google-cla Bot added the cla: yes This human has signed the Contributor License Agreement. label Aug 10, 2021

product-auto-label Bot added the samples Issues that are directly related to samples. label Aug 10, 2021

jskeet approved these changes Aug 10, 2021

View reviewed changes

Sita04 reviewed Aug 11, 2021

View reviewed changes

Comment thread compute/api/Compute.Samples.Tests/ListImagesAsyncTest.cs Outdated

Sita04 reviewed Aug 11, 2021

View reviewed changes

Sita04 requested changes Aug 11, 2021

View reviewed changes

rsamborski approved these changes Aug 12, 2021

View reviewed changes

Comment thread compute/api/Compute.Samples.Tests/ListImagesAsyncTest.cs Outdated

Comment thread compute/api/Compute.Samples.Tests/ListImagesPerPageAsyncTest.cs Outdated

amanda-tarafa force-pushed the compute branch from 95501d6 to 8ba8eff Compare August 13, 2021 08:09

amanda-tarafa commented Aug 13, 2021

View reviewed changes

amanda-tarafa added 2 commits August 13, 2021 12:50

feat(Compute): Use paginated lists.

0d0c81c

Updates Google.Cloud.Compute.V1 dependency. Closes GoogleCloudPlatform#1438

feat(Compute): Add Images listing samples.

9e1946a

amanda-tarafa force-pushed the compute branch from 8ba8eff to 9e1946a Compare August 13, 2021 11:51

Sita04 approved these changes Aug 13, 2021

View reviewed changes

amanda-tarafa merged commit b6c2e98 into GoogleCloudPlatform:master Aug 16, 2021

amanda-tarafa deleted the compute branch August 16, 2021 10:40

-        // Calling AsRawResponses will return a sequence of pages.
+      // Use the `AsRawResponses` attribute of returned iterable to have more granular control of
+      // iteration over paginated results from the API. Each time you want to access the
+      // next page, the library retrieves that page from the API.

Uh oh!

Conversation

amanda-tarafa commented Aug 10, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snippet-bot Bot commented Aug 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amanda-tarafa commented Aug 11, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sita04 left a comment

Choose a reason for hiding this comment

Uh oh!

rsamborski left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

amanda-tarafa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sita04 commented Aug 13, 2021

Uh oh!

amanda-tarafa commented Aug 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

snippet-bot Bot commented Aug 11, 2021 •

edited

Loading

rsamborski left a comment •

edited

Loading